Remove the older collector stress-test setting, now that the newer one covers it - #7741
Conversation
📝 WalkthroughWalkthroughThis PR retires GC zeal controls and replaces them with deterministic seeded scheduling. It adds allocation pacing, evacuation and liveness instrumentation, poll-arm handling, diagnostics ownership, smoke validation, schedule fuzzing, and updated documentation and tests. ChangesSeeded GC schedule
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
096c2a1 to
1c82a54
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/gc/mod.rs (1)
992-1037: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGate the liveness verdict on the main thread, as the exit summary already is.
js_gc_release_current_thread_collection_side_allocationsruns on every thread teardown.schedule::report_exit_summary()guards against non-final counters withis_main_thread_or_unrecorded()andSUMMARY_EMITTED.emit_schedule_liveness_verdict()has no such guard.Under
PERRY_GC_SCHEDULE_RATE=1, a worker thread that tears down before the main thread reads the process-global counters early. If the schedule has not yet forced a collection on any thread,schedule_verdictreturnsErrandstd::process::exit(70)terminates the whole process. The run then reports a vacuous-instrument failure that did not occur.Calling
std::process::exitfrom a worker thread also runsatexithandlers while other threads still execute.Apply the same main-thread and once-only gate.
🐛 Proposed fix
fn emit_schedule_liveness_verdict() { + // Same ownership rule as `report_exit_summary`: the counters are + // process-global, so only the thread that tears down last may judge them. + if !crate::native_handle::is_main_thread_or_unrecorded() { + return; + } match schedule_liveness_report() { None => {} Some(Ok(summary)) => eprintln!("{summary}"), Some(Err(complaint)) => { eprintln!("{complaint}"); std::process::exit(70); } } }Based on learnings: record the thread that installs the failure reporter as the runtime main thread and ensure it owns the once-only exit summary; keep the unrecorded-main-thread fallback limited to paths where the GC schedule never activates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/gc/mod.rs` around lines 992 - 1037, Gate emit_schedule_liveness_verdict() behind the same main-thread and once-only ownership rules as schedule::report_exit_summary(), so worker-thread teardown cannot evaluate the verdict or call process exit. Record the thread installing the failure reporter as the runtime main thread, and keep the unrecorded-main-thread fallback only when the GC schedule never activates. Ensure the main thread owns the single exit-summary/verdict emission.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/lower_call/extern_timers.rs`:
- Line 35: Add PERRY_GC_SCHEDULE_SEED with a valid deterministic value to the
reproduction command in the documentation comment near the extern-timers
context, alongside PERRY_GC_SCHEDULE_RATE and PERRY_GC_PROTECT_FROMSPACE, so
scheduled collections are actually enabled and the moving-GC case remains
reproducible.
In `@crates/perry-runtime/src/gc/policy.rs`:
- Around line 2504-2517: Update gc_safepoint_moving_minor to return whether it
actually handled the safepoint, returning false on every blocked or early-exit
condition and true after successful handling. In the polling flow around
schedule_poll_collection_due, only call note_schedule_poll_collection with the
post-safepoint allocation level when the returned handled state is true;
preserve the existing paced path without consuming a schedule slot when handling
is blocked.
In `@crates/perry-runtime/src/gc/schedule.rs`:
- Around line 425-429: Update parse_schedule_alloc_kb so parsed allocation
values that would overflow or exceed a sane configured maximum are clamped or
rejected rather than converted to usize::MAX; preserve the default stride for
invalid or rejected input and ensure schedule_poll_collection_due continues
producing valid poll decisions.
In `@crates/perry-runtime/src/gc/tests/schedule.rs`:
- Around line 192-230: Protect schedule counter baselines, updates, and delta
assertions in the affected test with the shared lock held by every test that
uses these counters, rather than relying only on CopyingNurseryTestGuard. Update
the test setup around gc_schedule_safepoints() and
gc_schedule_forced_collections() to acquire the common lock, or reset both
counters to zero under that lock, so assertions such as safepoints_before + 1
cannot observe parallel-test increments.
In `@crates/perry-runtime/src/gc/tests/triggers.rs`:
- Around line 867-885: Update
a_resolved_seed_holds_the_poll_word_armed_with_nothing_pending to invoke
poll_arm::resolve_poll_seed (or js_gc_loop_safepoint) while ScheduleGuard::set
is active, then assert the poll remains armed after that call rather than
relying on startup state. Add a complementary test or branch using
ScheduleGuard::off() that invokes the same resolution path and verifies the seed
is released, covering both schedule-enabled and disabled call_once behavior.
In `@docs/src/internals/memory-model.md`:
- Around line 147-153: Correct the 600-poll example in the memory-model
documentation: do not imply that PERRY_GC_SCHEDULE_RATE=1 alone triggers
collection on every loop back-edge poll. Add PERRY_GC_SCHEDULE_ALLOC_KB=0 to the
example to explicitly enable every-poll collection, or revise the wording to say
collections occur per eligible candidate while retaining the documented default
allocation pacing.
In `@scripts/gc_schedule_fuzz.sh`:
- Around line 80-85: Update RATE handling in the script so unsupported values
are rejected or normalized before execution, and use that effective value
consistently in the run summary and reproduce command. Ensure the seeded arm
does not inherit PERRY_GC_SCHEDULE_ALLOC_KB in default mode; otherwise include
its effective value in both the summary and reproduce command. Apply the same
consistency to the schedule reporting and reproduction logic around the
referenced sections.
---
Outside diff comments:
In `@crates/perry-runtime/src/gc/mod.rs`:
- Around line 992-1037: Gate emit_schedule_liveness_verdict() behind the same
main-thread and once-only ownership rules as schedule::report_exit_summary(), so
worker-thread teardown cannot evaluate the verdict or call process exit. Record
the thread installing the failure reporter as the runtime main thread, and keep
the unrecorded-main-thread fallback only when the GC schedule never activates.
Ensure the main thread owns the single exit-summary/verdict emission.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9434c9d6-2c46-4259-a4c8-65a18584f025
📒 Files selected for processing (43)
.github/workflows/test.ymlCLAUDE.mdchangelog.d/7317-seeded-gc-schedule-fuzzing.mdchangelog.d/7741-retire-gc-zeal-for-the-seeded-schedule.mdcrates/perry-codegen/src/lower_call/extern_timers.rscrates/perry-codegen/src/stmt/loops.rscrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/instruments.rscrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/poll_arm.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/copying/deferred_finalize_7635.rscrates/perry-runtime/src/gc/tests/evacuation.rscrates/perry-runtime/src/gc/tests/fromspace_protect.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/runtime_roots/fs_options_object.rscrates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rscrates/perry-runtime/src/gc/tests/runtime_roots/json_shape_template.rscrates/perry-runtime/src/gc/tests/runtime_roots/symbol_description.rscrates/perry-runtime/src/gc/tests/schedule.rscrates/perry-runtime/src/gc/tests/triggers.rscrates/perry-runtime/src/gc/zeal.rscrates/perry-runtime/src/iterator_helpers.rscrates/perry-runtime/src/native_handle.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/path/value_args.rsdocs/engine-plan.mddocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mddocs/src/internals/rfc-rooting-by-construction.mddocs/statepoint-gc-experiment.mdrun_parity_tests.shscripts/gc_instrument_smoke.shscripts/gc_schedule_fuzz.shtest-files/test_gap_7564_iter_result_rooting.tstest-files/test_gap_gc_call_argument_rooting.tstest-files/test_gap_gc_regexp_receiver_rooting.tstest-files/test_gap_gc_rest_argument_rooting.tstest-files/test_gap_gc_same_module_call_argument_rooting.tstest-parity/gc_repsel_corpus.txt
💤 Files with no reviewable changes (1)
- crates/perry-runtime/src/gc/zeal.rs
jdalton
changed the title
PERRY_GC_ZEAL for the seeded schedule
✅ Action performedComments resolved. Approval is disabled; enable |
1616552 to
8dfec13
Compare
PERRY_GC_SCHEDULE_RATE=1 selects every candidate safepoint, and PERRY_GC_SCHEDULE_ALLOC_KB carries the allocation pacing (PerryTS#7728) with the same 4 KB default and the same 0 escape hatch, so the removed knob has no behaviour the schedule cannot reach. Two knobs differing only in how they pick safepoints are two configurations to keep exercised, and keeping both had already forced a precedence rule to stop their counters double-counting one minor. The PerryTS#7604 liveness counters move to gc/instruments.rs -- they count what the collector did, not what forced it -- and feed both the schedule's exit summary and schedule_liveness_report, which keeps the verdict's three causes and its exit 70 at the rate-1 endpoint. Below the endpoint a sampling run that forces nothing is legitimate (RATE=0 is the control arm), so no verdict is issued there. Every removed test has a schedule counterpart; three were dropped as exact duplicates. The smoke script's two stress arms run against the schedule with their non-vacuity assertions intact. Review follow-ups included: a blocked safepoint no longer charges the pacing stride, PERRY_GC_SCHEDULE_ALLOC_KB clamps rather than saturating into an off switch, the poll-word arming test drives the real resolution in both directions, and gc_schedule_fuzz.sh rejects an out-of-range rate instead of printing a reproduce command for a density it did not run at.
…down abort Rebase resolution: main's landed PerryTS#7782 ScheduleGuard (poll-word arming) and PerryTS#7737/PerryTS#7742 policy content are kept; the branch's schedule pacing, verdict, instruments split and doc rewrites apply on top. Audit found every seeded run aborting at exit (worker teardown + std::thread::current after TLS destruction) — fixed via an OS-id diagnostics-owner gate, once-guarded verdict, and a regression test. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
8dfec13 to
097df0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test-parity/gc_repsel_corpus.txt (2)
750-750: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a compound modifier for
zero-copying.Change
zero copying minorstozero-copying minorsto make the GC condition unambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-parity/gc_repsel_corpus.txt` at line 750, Update the comment near the low-allocation read loop to hyphenate “zero copying” as “zero-copying,” yielding “zero-copying minors” without changing the surrounding meaning.Source: Linters/SAST tools
746-746: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
zealreference.This entry still says
15,843 objects moved under zeal at audit time. Replace it with schedule-neutral wording, such asduring the audit run. The PR objective removes GC zeal controls and remaining references outside historical changelogs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-parity/gc_repsel_corpus.txt` at line 746, Update the corpus entry containing “15,843 objects moved under zeal at audit time” to remove the stale “zeal” reference, replacing it with schedule-neutral wording such as “during the audit run.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 318: Revert the version change in the workspace package configuration,
restoring the prior version value and leaving the existing PR-keyed changelog
fragment unchanged.
In `@crates/perry-runtime/src/native_handle.rs`:
- Around line 89-103: Update install_failure_reporter() to call
record_diagnostics_owner_thread() before installing the exit reporter, ensuring
the thread activating schedule reporting owns the once-only teardown
diagnostics.
---
Outside diff comments:
In `@test-parity/gc_repsel_corpus.txt`:
- Line 750: Update the comment near the low-allocation read loop to hyphenate
“zero copying” as “zero-copying,” yielding “zero-copying minors” without
changing the surrounding meaning.
- Line 746: Update the corpus entry containing “15,843 objects moved under zeal
at audit time” to remove the stale “zeal” reference, replacing it with
schedule-neutral wording such as “during the audit run.”
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38744bf6-3490-4a2d-b769-1983aca0873e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/test.ymlCLAUDE.mdCargo.tomlchangelog.d/7741-retire-gc-zeal-for-the-seeded-schedule.mdcrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/promote_in_place.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/evacuation.rscrates/perry-runtime/src/gc/tests/schedule.rscrates/perry-runtime/src/gc/tests/triggers.rscrates/perry-runtime/src/native_handle.rscrates/perry-runtime/src/object/class_registry/construct.rsdocs/src/internals/memory-model.mdtest-parity/gc_repsel_corpus.txt
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/perry-runtime/src/gc/tests/evacuation.rs
- crates/perry-runtime/src/gc/copying.rs
- .github/workflows/test.yml
- crates/perry-runtime/src/object/class_registry/construct.rs
- crates/perry-runtime/src/gc/layout.rs
- crates/perry-runtime/src/gc/tests/schedule.rs
- docs/src/internals/memory-model.md
- crates/perry-runtime/src/gc/mod.rs
- CLAUDE.md
- crates/perry-runtime/src/gc/policy.rs
- crates/perry-runtime/src/gc/schedule.rs
|
|
||
| [workspace.package] | ||
| version = "0.5.1457" | ||
| version = "0.5.1458" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the workspace version change.
This PR already includes the required PR-keyed changelog fragment. Revert this version edit. Maintainers apply the release version during merge or release work.
As per coding guidelines, external contributor PRs must not modify [workspace.package].version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` at line 318, Revert the version change in the workspace package
configuration, restoring the prior version value and leaving the existing
PR-keyed changelog fragment unchanged.
Sources: Coding guidelines, Learnings
| /// Record the CALLING thread as the owner of teardown-path once-only | ||
| /// diagnostics, first caller wins. Separate from [`runtime_main_thread_id`]'s | ||
| /// capture on purpose: that one races among every handle-creating thread, so | ||
| /// piggy-backing the OS id on its winning arm leaves the word 0 whenever a | ||
| /// handle call recorded main first — and a 0 here reads as "unrecorded", | ||
| /// which waves EVERY thread through [`is_main_thread_or_unrecorded`] and | ||
| /// reintroduces the worker-teardown print this exists to prevent. | ||
| pub(crate) fn record_diagnostics_owner_thread() { | ||
| let _ = MAIN_OS_THREAD_ID.compare_exchange( | ||
| 0, | ||
| current_os_thread_id(), | ||
| Ordering::AcqRel, | ||
| Ordering::Acquire, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Register the diagnostics owner when the schedule installs reporting.
crates/perry-runtime/src/gc/schedule.rs:592-610 calls only runtime_main_thread_id(). It does not call record_diagnostics_owner_thread(). Therefore MAIN_OS_THREAD_ID stays zero after schedule activation. Lines 114-118 then allow every teardown thread through the exit-summary gate.
Call record_diagnostics_owner_thread() in install_failure_reporter() before installing the exit reporter.
Proposed fix
fn install_failure_reporter() {
if REPORTER_INSTALLED.swap(true, Ordering::SeqCst) {
return;
}
crate::native_handle::runtime_main_thread_id();
+ crate::native_handle::record_diagnostics_owner_thread();
let previous = std::panic::take_hook();Based on learnings, record the thread that installs the failure reporter as the runtime main thread and ensure it owns the once-only exit summary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/native_handle.rs` around lines 89 - 103, Update
install_failure_reporter() to call record_diagnostics_owner_thread() before
installing the exit reporter, ensuring the thread activating schedule reporting
owns the once-only teardown diagnostics.
Source: Learnings
|
Audit complete — merging. This closes out the GC knob kill-policy's own demand: with the seeded schedule strictly subsuming zeal, the older mode was "a decision that hasn't been made", and this PR makes it. The maintainer's precondition —
Rebase notes (the branch predated four merged PRs on the same files): main's landed #7782 Found and fixed during audit — a worker-teardown abort under ANY resolved seed (exit 134 after correct output): the exit summary runs on the per-thread teardown funnel, where a tokio worker's TLS is already destroyed; the main-thread gate called Verified: 2,051 runtime tests green; pacing sabotage ( |
…7717) (#7793) * test(gc): dependency-scale runtime witness for the moving collector (#7717) * port the witness from PERRY_GC_ZEAL to the rate-1 seeded schedule (#7741) Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * chore: bump version to 0.5.1460 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
2 participants
Removing the older collector stress-test setting
Perry ended up with two environment variables that do nearly the same job: force the garbage collector to run far more often than normal, so that memory bugs which normally hide behind lucky timing get shaken out. This pull request keeps the newer one and deletes the older one.
Stacked on #7317, which adds the newer one. Until that merges this branch's diff will include its commit too; I will rebase once it lands so this collapses to just the removal.
What goes away
PERRY_GC_ZEALand its companionPERRY_GC_ZEAL_ALLOC_KBare removed. Everything they did is available throughPERRY_GC_SCHEDULE_SEED:PERRY_GC_SCHEDULE_RATE=1collects at every opportunity — the same moments the old setting collected at.PERRY_GC_SCHEDULE_ALLOC_KBcarries over the old pacing behaviour unchanged, with the same 4 KB default and the same0value meaning "collect at literally every opportunity".So there is no configuration of the old setting that the new one cannot express.
Why remove it instead of keeping both
Two settings that differ only in how they choose moments to collect are two configurations somebody has to keep testing. The project's own policy in
CLAUDE.mdis blunt about this: a mode that still exists is a decision nobody has made. The repository has repeatedly paid for options that quietly stopped being exercised and turned out to have been broken for months.Keeping both had already started to cost something concrete. Because either setting could force the same collection, they needed a rule about which one got to claim it, so their counters would not both count it and make the totals meaningless. That rule exists purely to reconcile a redundancy — it buys nothing except the ability to keep two overlapping settings.
And the newer one is strictly more capable. It covers normal collection frequency at one end, the old setting's maximum frequency at the other, and everything in between — and unlike the old one, when it finds a bug it hands you a number that replays the failure.
The safety checks from the old setting all survive the move
The old setting had picked up a layer of self-checking, and none of it is lost.
There were three counters that record what a run actually did: how many collections moved objects, how many objects moved, and how many collection opportunities the program reached. These now live in
gc/instruments.rs. That is a deliberate relocation rather than a rename — they measure what the collector did, not what asked it to, so putting them inside any one setting's file is how a counter ends up being deleted along with the next setting that happens to carry it. They now also appear in the exit summary, so even a run at a low collection rate reports whether it exercised anything.There was also a verdict at exit that fails the process outright when the run turns out to have done nothing — the point being that "no failures" from a run that never actually collected is not evidence of anything. That verdict survives with the same three failure reasons and the same exit code.
One change worth flagging: the verdict now only applies at the maximum rate. That is deliberate. At the maximum rate, a run that collected nothing genuinely is broken. But at a sampling rate, a run that happens to collect nothing is a perfectly legitimate outcome — a rate of 0 is the documented "on but selects nothing" control, and a sweep seed that fires late is not a malfunction. Applying the strict verdict there would turn every quiet seed into a false alarm.
Test coverage moved rather than shrank
Every test the old setting had now has an equivalent: the pacing limit and its two checks that pacing did not simply switch collection off, the escape hatch that restores collecting at every opportunity, the rule that pacing measures from after each collection so a collection that frees nothing cannot loop forever, the guarantee that the fast-path check stays enabled so the mode cannot silently become a no-op, the guarantee that a collection moves objects, and the check that the collector's stress mode and the memory-protection tooling work together.
Three tests were deleted outright because they were exact duplicates of tests that already existed for the newer setting.
The two end-to-end checks in
scripts/gc_instrument_smoke.sh— one pairing stress with the evacuation verifier, one enforcing that the whole thing still finishes in reasonable time on a realistic workload — now run against the newer setting with their non-vacuity assertions intact. Neither can report success from a run that collected or moved nothing.How this was verified
cargo check -p perry-runtime -p perry-codegen --all-targetsreports no errors. The warnings that remain are pre-existing onmainand are byte-for-byte identical there.scripts/check_file_size.shpasses, andbash -nreports the smoke script is syntactically clean.changelog.d/, where past entries are a historical record and are left untouched.No version bump — the maintainer bumps that at merge time, per the external-contributor flow.
Summary by CodeRabbit